Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[BUG_FIX] Fix resize test #6298

Merged
merged 10 commits into from
Aug 22, 2020
Merged

[BUG_FIX] Fix resize test #6298

merged 10 commits into from
Aug 22, 2020

Conversation

electriclilies
Copy link
Contributor

This PR fixes the bug in #6237. In resize, the method "nearest_neighbor" is not compatible with the coordinate_transform_mode "align_corners", and the method "bilinear" is not compatible with coordinate_transformation_modes other than "align_corners".

I also added checks to the topi resize method to prevent the user from passing incompatible method and coordinate_transformation_mode combinations into resize.

@mbrookhart @masahi please take a look!

@electriclilies electriclilies mentioned this pull request Aug 18, 2020
Closed
@masahi masahi self-assigned this Aug 18, 2020
mbrookhart
mbrookhart requested changes Aug 18, 2020
View reviewed changes
Copy link
Contributor

@mbrookhart mbrookhart left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add another test case? I'm wondering about the combinations of the various modes, the check is missing any reference to half_pixel and bicubic.

tests/python/relay/test_op_level5.py Outdated Show resolved Hide resolved
python/tvm/topi/image/resize.py Show resolved Hide resolved
mbrookhart
mbrookhart approved these changes Aug 18, 2020
View reviewed changes
Copy link
Contributor

@mbrookhart mbrookhart left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

test_dynamic_to_static_resize also needs to be updated, otherwise looks great

@masahi
Copy link
Member

masahi commented Aug 19, 2020 •
edited
Loading

Frontend tests are failing really bad with this change. We need to fix hardcoded
https://github.com/apache/incubator-tvm/blob/0a4ee30da727ae1566a0541ba6ed726d48733cc7/python/tvm/relay/frontend/onnx.py#L935

There is also bilinear + align_corners=False combination in tflite test, which translates to bilinear + asymmetric in Relay:
https://github.com/apache/incubator-tvm/blob/77042327d944fbfa243f59740c087d433bc1569e/tests/python/frontend/tflite/test_forward.py#L1058

I don't know what TF does with bilinear + align_corners=False. Also not sure if bilinear + asymmetric in Relay is a valid combination. Maybe @jwfromm @srkreddy1238 have thought on this?

@electriclilies electriclilies marked this pull request as draft August 19, 2020 19:46
@electriclilies
Copy link
Contributor Author

@masahi I changed the onnx frontend to set align_corners to False if the method is 'nearest_neighbor', otherwise set it to true. This got rid of the issue in the onnx importer.

TFLite is not passing one test, though, because it's trying to use the bilinear + asymmetric combination...

@electriclilies
Copy link
Contributor Author

It looks like bilinear + asymmetric passes the resize test, but for some reason it is not tested in the topi test. I added it to the topi test and removed that check from the topi resize op.

@masahi
Copy link
Member

masahi commented Aug 19, 2020

Yes this change now looks good to me. After CI pass we can merge.

@electriclilies
Copy link
Contributor Author

@masahi Thanks!

@electriclilies electriclilies marked this pull request as ready for review August 19, 2020 22:29
@masahi
Copy link
Member

masahi commented Aug 20, 2020

oh CI is failing because there is nearest + half_pixel combination in pytorch frontend ...

https://github.com/apache/incubator-tvm/blob/939a42b4e976a41e8513b720421d3c3678493715/python/tvm/relay/frontend/pytorch.py#L1542-L1545

@electriclilies can you fix the condition above to

        if method == "nearest_neighbor":
            coord_trans = "asymmetric"
        elif align_corners:
            coord_trans = "align_corners"
        else:
            coord_trans = "half_pixel"

Options in resize is a hairy issue, sorry to have you go through this.

@ZihengJiang ZihengJiang added the status: need update need update based on feedbacks label Aug 21, 2020
@electriclilies
Copy link
Contributor Author

electriclilies commented Aug 21, 2020 •
edited
Loading

@masahi Thanks! it looks like there is a similar condition in upsample3d, I also changed that for consistency. Let me know if you think that will cause a problem

@masahi masahi merged commit 9a0413a into apache:master Aug 22, 2020
@masahi
Copy link
Member

masahi commented Aug 22, 2020

Thanks @electriclilies @mbrookhart

trevor-m pushed a commit to trevor-m/tvm that referenced this pull request Aug 26, 2020
@electriclilies
[BUG_FIX] Fix resize test (apache#6298)
469ae67 
* fix resize tests

* add different scale to resize tests

* fix dynamic to static resize test

* fix error throwing in topi resize

* fix topi and importer tests

* fix lint

* flakey test failed

* make resize test less sensitive; had floating point rounding err on gpu

* remove nearest_neighbor + half_pixel option from pytorch importer

* remove nearest_neighbor + half_pixel in upsample3d
trevor-m pushed a commit to trevor-m/tvm that referenced this pull request Aug 26, 2020
@electriclilies
[BUG_FIX] Fix resize test (apache#6298)
b6a15a7 
* fix resize tests

* add different scale to resize tests

* fix dynamic to static resize test

* fix error throwing in topi resize

* fix topi and importer tests

* fix lint

* flakey test failed

* make resize test less sensitive; had floating point rounding err on gpu

* remove nearest_neighbor + half_pixel option from pytorch importer

* remove nearest_neighbor + half_pixel in upsample3d
trevor-m pushed a commit to trevor-m/tvm that referenced this pull request Aug 26, 2020
@electriclilies
[BUG_FIX] Fix resize test (apache#6298)
ccc4e5d 
* fix resize tests

* add different scale to resize tests

* fix dynamic to static resize test

* fix error throwing in topi resize

* fix topi and importer tests

* fix lint

* flakey test failed

* make resize test less sensitive; had floating point rounding err on gpu

* remove nearest_neighbor + half_pixel option from pytorch importer

* remove nearest_neighbor + half_pixel in upsample3d
trevor-m pushed a commit to trevor-m/tvm that referenced this pull request Sep 2, 2020
@electriclilies
[BUG_FIX] Fix resize test (apache#6298)
b412592 
* fix resize tests

* add different scale to resize tests

* fix dynamic to static resize test

* fix error throwing in topi resize

* fix topi and importer tests

* fix lint

* flakey test failed

* make resize test less sensitive; had floating point rounding err on gpu

* remove nearest_neighbor + half_pixel option from pytorch importer

* remove nearest_neighbor + half_pixel in upsample3d
trevor-m pushed a commit to neo-ai/tvm that referenced this pull request Sep 3, 2020
@electriclilies @trevor-m
[BUG_FIX] Fix resize test (apache#6298)
acc81fd 
* fix resize tests

* add different scale to resize tests

* fix dynamic to static resize test

* fix error throwing in topi resize

* fix topi and importer tests

* fix lint

* flakey test failed

* make resize test less sensitive; had floating point rounding err on gpu

* remove nearest_neighbor + half_pixel option from pytorch importer

* remove nearest_neighbor + half_pixel in upsample3d
@ZihengJiang ZihengJiang mentioned this pull request Sep 25, 2020
Closed
@electriclilies electriclilies mentioned this pull request Feb 25, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mbrookhart mbrookhart mbrookhart approved these changes

@masahi masahi masahi approved these changes

Assignees

@masahi masahi

Labels
status: need update need update based on feedbacks
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@electriclilies @masahi @mbrookhart @ZihengJiang